Skip to content

Apply the safe misc-const-correctness fixes - #275

Merged
helly25 merged 2 commits into
mainfrom
clang_tidy_const_correctness
Aug 8, 2026
Merged

Apply the safe misc-const-correctness fixes#275
helly25 merged 2 commits into
mainfrom
clang_tidy_const_correctness

Conversation

@helly25

@helly25 helly25 commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Fifth clang-tidy triage PR. 31 const additions across 17 files — all bare const, no logic changed.

The headline: this check's --fix is not safe here

I did not take the automatic fixes wholesale, and that turned out to matter. Applying all of them broke the build in three distinct ways:

file failure
mbo/types/stringify.h:916 cannot assign to variable 'idx' with const-qualified type — the variable is assigned. The check was simply wrong.
mbo/types/internal/struct_names_clang.h:127 a non-const reference can no longer bind to the now-const value
optional_ref_test.cc, optional_data_or_ref_test.cc static assertion failedconst changes the deduced type the tests assert on

mbo/hash/hash_benchmark.cc is the clearest case. It proposed const for:

total_bytes += static_cast<int64_t>(key.size());   // proposed const
benchmark::DoNotOptimize(Algo::GetHash64(keys[counter++ & ...]));  // proposed const

These are template-heavy sites where the mutating instantiation is not visible to the translation unit that exported the fix. Anyone running clang-tidy --fix for this check on this repo will silently break it.

Those five files are reverted here and keep 18 findings, to be handled individually rather than mechanically.

Method

Fixes were exported per TU with --export-fixes and merged once via clang-apply-replacements, rather than running --fix in parallel. With 81 TUs and headers included by many of them, parallel --fix would have had several processes rewriting the same header concurrently.

Test

  • bazel test --config=clang //...109/109 pass.
  • Every surviving change verified to be a bare const addition (git diff contains no other kind of line).
  • pre-commit run -a green.

Follow-up

The remaining 18 findings live in stringify.cc (1), hash_benchmark.cc (6), struct_names_test.cc (4), optional_ref_test.cc (1), optional_data_or_ref_test.cc (6). Since several are demonstrably wrong rather than merely awkward, NOLINT with a per-site reason looks right — but that is a judgement call per site, so it is left out of this PR.

31 const additions across 17 files, all of them locals that are genuinely
never modified.

The check's automatic fixes are NOT safe on this codebase and were not
taken wholesale. Applying all of them broke the build in three distinct
ways:

  * mbo/types/stringify.h:916 - "cannot assign to variable 'idx' with
    const-qualified type". The variable is assigned; the check was simply
    wrong.
  * mbo/types/internal/struct_names_clang.h:127 - a non-const reference
    can no longer bind to the now-const value.
  * mbo/types/optional_ref_test.cc, optional_data_or_ref_test.cc - static
    assertions fail, because const changes the deduced type the tests
    assert on.

mbo/hash/hash_benchmark.cc is the clearest case: it proposed const for
`total_bytes`, which is `+=`-accumulated, and `counter`, which is
`counter++`-incremented. These are template-heavy sites where the
mutating instantiation is not visible to the translation unit that
exported the fix.

Those five files are reverted and keep 18 findings, to be handled
individually rather than by --fix. Everything here is a bare `const`
addition; no logic changed.

Fixes were exported per TU and merged with clang-apply-replacements
rather than applying --fix in parallel, so that headers shared by many
TUs could not be written concurrently.

bazel test --config=clang //... - 109/109 pass.

Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
@helly25
helly25 requested a review from Fab-Cat August 8, 2026 20:22
@helly25
helly25 enabled auto-merge (squash) August 8, 2026 20:23
@helly25
helly25 merged commit 8ef9d95 into main Aug 8, 2026
23 checks passed
@helly25
helly25 deleted the clang_tidy_const_correctness branch August 8, 2026 20:56
helly25 added a commit that referenced this pull request Aug 9, 2026
#275 made `test` and `item` const in limited_set_benchmark.h, but both
are assigned:

  ./mbo/container/limited_set_benchmark.h:134:11: error: cannot assign to
  variable 'item' with const-qualified type 'const std::size_t'

It reached main because `bazel test //...` skips `manual` targets, so
nothing built the benchmark. Only #280 - which put those targets into the
compile DB - made clang-tidy able to see it. Reverted, and CI now builds
every clang-tidy-tagged manual target so the next one cannot slip through
the same way. All five build.

tools/clang_tidy.sh now skips mbo/hash/measurements/smhasher3/: bazel does
not build it (it is an SMHasher3 plugin compiled by that project's cmake),
so clang-tidy was linting it with flags guessed from unrelated files and
reporting its SMHasher3 includes as missing. That removes 4 spurious
categories at once, including all 10 c-style-cast findings.

Disabled, each with its reason recorded:
  * misc-no-recursion - recursion is the shape of the code it flags
    (Stringify walks nested structures, BigNumberLen recurses once for
    negatives), and it reports every member of a call chain.
  * bugprone-std-namespace-modification - the `namespace std` blocks are
    specialisations of std templates for our own types, which is how
    those extension points work.
  * cppcoreguidelines-avoid-magic-numbers / readability-magic-numbers
    (aliases) - they fire on test expectations and on the numeric tables
    that ARE the subject.

bazel test --config=clang //... - 109/109 pass.

Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25 added a commit that referenced this pull request Aug 9, 2026
Two of the sites were real and are fixed:
  * stringify.h - a loop `std::string_view` that is never mutated, which
    STYLE_CPP.md now says should be const.
  * optional_data_or_ref_test.cc - a `std::string` only ever read.

The rest are the check being wrong, so they get a NOLINT naming why:
  * struct_names_clang.h (4) - `field_index` and `storage` are passed to
    FieldCount()/Init() by non-const reference and via `&storage.Get()`.
    #275 showed const there fails to compile.
  * optional_ref_test.cc, optional_data_or_ref_test.cc (9) - these
    declarations are the subject under test. const changes the deduced
    type a neighbouring `static_assert(...decltype(ref)...)` checks.

misc-const-correctness now reports zero.

Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25 added a commit that referenced this pull request Aug 9, 2026
#275 made `test` and `item` const in limited_set_benchmark.h, but both
are assigned:

  ./mbo/container/limited_set_benchmark.h:134:11: error: cannot assign to
  variable 'item' with const-qualified type 'const std::size_t'

It reached main because `bazel test //...` skips `manual` targets, so
nothing built the benchmark. Only #280 - which put those targets into the
compile DB - made clang-tidy able to see it. Reverted, and CI now builds
every clang-tidy-tagged manual target so the next one cannot slip through
the same way. All five build.

tools/clang_tidy.sh now skips mbo/hash/measurements/smhasher3/: bazel does
not build it (it is an SMHasher3 plugin compiled by that project's cmake),
so clang-tidy was linting it with flags guessed from unrelated files and
reporting its SMHasher3 includes as missing. That removes 4 spurious
categories at once, including all 10 c-style-cast findings.

Disabled, each with its reason recorded:
  * misc-no-recursion - recursion is the shape of the code it flags, and
    it reports every member of a call chain.
  * bugprone-std-namespace-modification - the `namespace std` blocks are
    specialisations of std templates for our own types.
  * cppcoreguidelines-avoid-magic-numbers / readability-magic-numbers
    (aliases) - they fire on test expectations and on the numeric tables
    that ARE the subject.

bazel test --config=clang //... - 109/109 pass.

Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25 added a commit that referenced this pull request Aug 9, 2026
Two of the sites were real and are fixed:
  * stringify.h - a loop `std::string_view` that is never mutated, which
    STYLE_CPP.md now says should be const.
  * optional_data_or_ref_test.cc - a `std::string` only ever read.

The rest are the check being wrong, so they get a NOLINT naming why:
  * struct_names_clang.h (4) - `field_index` and `storage` are passed to
    FieldCount()/Init() by non-const reference and via `&storage.Get()`.
    #275 showed const there fails to compile: "binding reference of type
    'std::size_t' to value of type 'const std::size_t' drops 'const'".
  * optional_ref_test.cc, optional_data_or_ref_test.cc (9) - these
    declarations are the subject under test. Adding const changes the
    deduced type a neighbouring `static_assert(IsOptionalRef<decltype(ref)>)`
    checks, and changes which overloads the test exercises. #275 showed
    those static assertions failing.

misc-const-correctness now reports zero.
bazel test --config=clang //... - 109/109 pass.

Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25 added a commit that referenced this pull request Aug 9, 2026
Applied clang-tidy's fixes for readability-math-missing-parentheses,
modernize-type-traits, modernize-use-auto, modernize-use-constraints and
readability-static-definition-in-anonymous-namespace: explicit precedence
parentheses, `std::remove_reference_t<T>` for
`std::remove_reference<T>::type`, `auto` for a repeated cast type, and
`requires` clauses in place of `std::enable_if_t` parameters.

readability-redundant-parentheses is DISABLED, because its fix corrupts
code rather than tidying it. On

  __builtin_dump_struct(ptr, &DumpStructVisitor, fields, field_index);

it deletes the callee - exactly the 21 characters of the builtin's name -
leaving

  (ptr, &DumpStructVisitor, fields, field_index);

a comma expression that does nothing and fails to compile under
-Werror,-Wunused-value. It did this in struct_names_clang.h and
extend_test.cc, i.e. the reflection machinery. Both files were reverted.
The findings it reports are cosmetic; the hazard of anyone running --fix
is not.

That is the third check whose auto-fix is unsafe here, after
misc-const-correctness (#275, wrong const on assigned variables) and
misc-use-internal-linkage (#284, static on header-declared functions).

bazel test --config=clang //... - 109/109 pass.

Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants